Repository navigation
fix(python): find blocked imports by parsing, not by line prefix - #2831
Conversation
The embedded Monty python3 blocked `import re` by matching lines that start with `import`/`from`. Any statement prefix bypassed it: `pass; import re`, `if True: import re`, an import inside a function, class, loop or try. Parse with the same ruff parser Monty runs first and walk every statement body instead.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
bashkit | 232c3ca | Commit Preview URL Branch Preview URL |
Nov 05 2026, 03:25 AM |
![bashkit-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/249470897?s=60&v=4){kind=small}
| // Monty has no `__import__` or `importlib`, so import statements are | ||
| // the only way to load a module. Pin that: if Monty adds either, the | ||
| // AST gate in `is_disabled_import` must learn about it. | ||
| let r = run(&["-c", "m = __import__('re')\nprint(m)"], None).await; |
There was a problem hiding this comment.
[P1] The exec/eval/compile dynamic path is not pinned. The gate is purely static (single check in execute(); the old bypass "loaded the module anyway" per knowledge/log.md, so there is no runtime enforcement), and the outer AST of exec('import re') contains no Stmt::Import node, so it sails through. This test pins __import__ and importlib but not exec("import re") / compile+exec, and the existing blackbox no_exec/no_compile tests assert only !stdout.contains("hacked"), which passes whether or not the call executes. Please add a regression test such as run(&["-c", "exec('import re')\nprint(re.search('a','a'))"]]) asserting re stays unusable (or assert explicitly that exec is unavailable), so a Monty upgrade that adds/changes exec fails loudly instead of silently reopening the gate.
| } | ||
| None | ||
|
|
||
| let parsed = ruff_python_parser::parse_module(code).ok()?; |
There was a problem hiding this comment.
[P2] Untrusted input is fully AST-parsed before any budget is charged: the caller runs this gate and only afterwards calls consume_budget_input/work (execute(), ~lines 517-526). A multi-MB -c string or script file gets a full parse_module before accounting. Consider consuming input budget (or enforcing a length cap) before parsing.
| # Same parser Monty uses (already in the tree through monty =0.0.19). Bashkit | ||
| # parses user code with it to find `import` statements; see | ||
| # `is_disabled_import` in builtins/python.rs. Keep in step with Monty's pin. | ||
| ruff_python_parser = { version = "=0.0.3", optional = true } |
There was a problem hiding this comment.
[P2] Nothing enforces the "keep in step with Monty's pin" invariant: .github/dependabot.yml ignores monty/monty-types (held at 0.0.19) but not these two crates, so a Dependabot bump of ruff_python_parser/ast past Monty's bundled 0.0.3 silently skews the gate's grammar from the executor's. Skew in the wrong direction (gate rejects what Monty accepts) turns into pass-through (parse_module(...).ok()? returns None) and reopens the hole. Please add the same ignore entries for ruff_python_parser/ruff_python_ast.
Review summary (head
|
Requested by Михайло · project thread
What changed
The embedded Monty
python3now findsimport reanywhere in the program. The gate parses the code with the same ruff parser Monty runs first and walks every statement body (functions, classes, loops,if,with,try,match). Strings and comments that mentionrestill run.Why
The gate only matched lines starting with
import/from, so any statement prefix loaded the disabled module:pass; import re,if True: import re,def f(): import re. Found while reviewing a downstream integration (harnx), which documentedpass; import rereachingre.splitat ~300 MiB RSS.Monty 0.0.19 has no
__import__orimportlib, so import statements are the only loaders; a unit test pins that so a Monty upgrade that adds them fails loudly.Before / After
Before (0.20.3 CLI):
After: both exit 1 with
python3: importing module 're' is disabled for security reasons (regex DoS risk). Unit test covers 10 bypass shapes, plus a negative test (re = 'import re', comment) that still runs; integration security test inpython_security_tests.rs.Risk
ruff_python_parser/ruff_python_ast=0.0.3, already in the tree through monty; no new crates inCargo.lock. Code that fails to parse is passed to Monty, which reports the same SyntaxError.Checklist
🤖 Generated with Claude Code